-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Check that PRs' CHANGELOG.md changes don't modify already-released content in CI
#8351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Check that PRs' CHANGELOG.md changes don't modify already-released content in CI
#8351
Conversation
58ba751 to
8b3d6ff
Compare
|
Recent violators of the check I'm adding here that I had to fix in #8407: |
|
Sigh, this check is running into issues with GitHub's default |
556a765 to
e686fa7
Compare
|
I have several PRs demonstrating that core logic works here:
These are present in the check's unit tests in There are some questions that are still open for this, but none are blocking until we make this check required:
|
c13a16a to
8f102f2
Compare
.github/workflows/changelog.yml
Outdated
| @@ -0,0 +1,42 @@ | |||
| name: Check `CHANGELOG.md` | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshedding for the file's name welcome!
.github/workflows/changelog.yml
Outdated
| changelog: | ||
| timeout-minutes: 1 | ||
|
|
||
| name: Check `CHANGELOG` for errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshedding for the job's name welcome!
06acbe0 to
7ea2757
Compare
|
From today's maintainers' meeting notes:
|
7ea2757 to
8d83828
Compare
8d83828 to
8554263
Compare
|
|
||
| #[cfg(test)] | ||
| mod test_hunks_in_a_released_section { | ||
| #[collapse_debuginfo(yes)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What dafaq is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a dissting!
When macros are used outside the crate defining them, this is the default. Within a crate, however, compiler errors spans, by default, will point to expanded code, not use sites. I felt it was nicer to do this from a testing standpoint, but it's not absolutely necessary, as the tests are working.
I just noticed we don't seem to have coverage for xtask unit tests in CI… 🤔
EDIT: That's not right, sigh.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| use pico_args::Arguments; | ||
| use xshell::Shell; | ||
|
|
||
| pub(crate) fn check_changelog(shell: Shell, mut args: Arguments) -> anyhow::Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a check to make sure your working state isn't dirty? Is this destructive to the working copy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might, but I don't know if it's worth handling the complexity. Could you tell me what scenario you're trying to prevent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Do changes
- "I wonder if I did the changelog right"
- cargo xtask changelog
- Where changes?
- enters rage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I think that case should be handled just fine by the user not specifying the <to_commit> positional argument, in which case the CHANGELOG.md in the filesystem is used. Does that sound like it works?
To more definitively answer your question, there's no mutation of the working copy happening here.
dfc1280 to
dc8340a
Compare
dc8340a to
c37e504
Compare
We've had recurring issues with PRs'
CHANGELOG.mdentries going stale, particularly when PRs are filed close to a regular release date. This PR tries to make this by creating a CI check that fails when a PR modifies the changelog and it detects changes outside the## Unreleasedsection header.This is an MVP that is not yet suitable as a required check, because there is no escape hatch for cases where we do want to change released changelog content. The intent is to enable this once some follow-up to add such an escape hatch has achieved consensus and is implemented.